-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bat: add meta.mainProgram
#246943
bat: add meta.mainProgram
#246943
Conversation
What are all the unrelated changes? |
5807a50
to
58667bf
Compare
Just a minor cleanup, nothing fancy.
|
Why? |
There are several compelling reasons to avoid using a Firstly, employing such a statement imports all the functions from Secondly, a There are additional considerations regarding this topic that fall outside my expertise, but I encourage you to post this question on discourse for a more comprehensive discussion. You can find a very good article about that on nix.dev. There is also this RFC: https://github.com/r-burns/nixos-rfcs/blob/rfc-inherit-as-list/rfcs/0110-inherit-as-list.md |
Unless it's in the nixpkgs docs it is stylistic preference, please revert it. |
Closing the PR. |
Oops, wrong window. |
Global |
58667bf
to
836d243
Compare
Follow up of #246386
836d243
to
01b647a
Compare
For me it is fine if further cleanup changes happen in this PR, however please put them in separate commits that do not have the unsuspecting commit message "add meta.mainProgram" |
I'm not going to try to argue here, for me
Just a remark on this, depending on the contextual scope you're looking at it, a But ok, I don't want to loose too much time on this, I've reverted the changes, please let's move on. |
@@ -16,7 +16,7 @@ rustPlatform.buildRustPackage rec { | |||
|
|||
src = fetchFromGitHub { | |||
owner = "sharkdp"; | |||
repo = pname; | |||
repo = "bat"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a specific reason for this change? repo = pname
seems to be a very common pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repo = pname;
is nice for DRY but creates a binding that goes too far.
See further information about this here: nix-community/nixpkgs-lint#21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK finalattrs hasn't been implemented for buildRustPackage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly not yet indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there isn't a reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is still valid, even without finalAttrs
pattern. If one needs to overrides pname
, it will also have an impact on the src
. This change is preventing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't ask you to revert it but frankly this change is pointless.
I don't want to loose too much time on this
I'd ask you to consider that other people may not want to loose too much time
on stuff like this either.
This PR could have been a one line diff adding mainProgram that could have been merged without discussion as soon as it passed ofborg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm dedicating my time for free for this project and every time I edit a PR, I'm trying to use what the "seniors" has been telling me for months when I was submitting my first PRs.
Constantly refining for a greater good the things that I'm "touching", I see that it is not very welcome, point noted, maybe I should avoid dedicating time to this community?
Related PR:
Description of changes
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)